Fix race condition when initializing HealthCheckedEndpointGroup#6188
Fix race condition when initializing HealthCheckedEndpointGroup#6188
Conversation
jrhee17
left a comment
There was a problem hiding this comment.
It seems like the test in question has failed again: https://github.com/line/armeria/actions/runs/14173631131/job/39703141791?pr=6188
...st/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java
Show resolved
Hide resolved
|
I noticed that the test failure in In this test, registering the endpoint is essentially part of the setup phase. However, the internal initialization of HealthCheckedEndpointGroup is asynchronous, and Initially, I thought calling Internally, To address this, I changed the approach: This ensures that the state propagation is explicitly controlled and prevents any premature callback from breaking the test setup. |
| endpointGroup.addListener(endpointsListener, false); | ||
| endpointsListener.accept(endpointGroup.endpoints()); |
There was a problem hiding this comment.
Can't we do this?
| endpointGroup.addListener(endpointsListener, false); | |
| endpointsListener.accept(endpointGroup.endpoints()); | |
| endpointGroup.addListener(endpointsListener, true); |
There was a problem hiding this comment.
it works ! I think Fix duplicate endpoint handling in HealthCheckedEndpointGroup
this change helps the new group include endpoints from the previous group temporarily, which avoids any brief disappearance during the transition.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6188 +/- ##
============================================
- Coverage 74.46% 74.08% -0.38%
- Complexity 22234 22992 +758
============================================
Files 1963 2061 +98
Lines 82437 86135 +3698
Branches 10764 11312 +548
============================================
+ Hits 61385 63815 +2430
- Misses 15918 16896 +978
- Partials 5134 5424 +290 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| assertThat(endpointGroup.endpoints().get(0).attrs().attr(EndpointAttributeKeys.DEGRADED_ATTR)) | ||
| .isFalse(); | ||
| } | ||
| final HealthCheckedEndpointGroup endpointGroup = new HealthCheckedEndpointGroup( |
There was a problem hiding this comment.
Question) Shouldn't the HealthCheckedEndpointGroup still be closed to close the ScheduledFuture at L539?
| import { SelectOption } from '../../lib/types'; | ||
| import DebugInputs from './DebugInputs'; | ||
|
|
||
| const stringifyHeaders = (headers: [string, string[]][]): string => |
Motivation
Fixes #6018
A race condition can occur when adding a listener immediately after creating a HealthCheckedEndpointGroup.
Because the endpoint group initialization is asynchronous, the listener may receive endpoints in an incomplete or intermediate state.
Modification
Explicitly await the completion of HealthCheckedEndpointGroup initialization using whenReady().join() before adding a listener.
This ensures the listener will always receive a fully initialized endpoint list, eliminating any race conditions.
Result
• Race condition is fixed.
• Listener always receives a fully initialized EndpointGroup, ensuring reliable and predictable behavior.